You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

Walkthrough

Replaces the import-only scan in react_on_rails:doctor with a broader "base package reference" scan that detects CommonJS require, Jest/Vitest mock helpers, and TypeScript declare module blocks; updates messaging, tests, and CHANGELOG accordingly.

Changes

Base Package Reference Detection Enhancement

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 I hop through files both near and far,
Jest mocks, require, and .d.ts I spar.
I sniff base-package traces, one by one,
Pointing Pro paths till the job is done. ✨

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

Greptile Summary

This PR extends the check_base_package_imports doctor check to detect two additional stale-module-specifier patterns that survive a migration from react-on-rails to react-on-rails-pro: Jest/Vitest mock calls (jest.mock('react-on-rails', ...)) and TypeScript ambient module declarations (declare module 'react-on-rails' { … }). Both patterns are covered by well-formed regex constants and backed by new RSpec contexts that exercise the detection end-to-end.

Confidence Score: 4/5

Safe to merge; the only finding is a P2 wording improvement to the warning message.

No logic errors or regressions introduced. The two new regex patterns are correct and the tests validate them. The only concern is a P2: the existing warning message and fix suggestion are framed around import/require patterns and don't give accurate guidance for the newly-detected mock-call and declare module cases.

react_on_rails/lib/react_on_rails/doctor.rb — warning message wording should be updated to mention mock/declare-module patterns.

Important Files Changed

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[check_base_package_imports] --> B[resolve_js_source_path]
    B --> C[Glob js/jsx/ts/tsx files]
    C --> D{File content matches?}
    D -->|import from react-on-rails| E[files_with_base_import << file]
    D -->|require react-on-rails| E
    D -->|jest.mock react-on-rails NEW| E
    D -->|declare module react-on-rails NEW| E
    D -->|no match| F[next file]
    E --> G{Any files collected?}
    G -->|yes| H[add_warning with file list]
    G -->|no| I[add_success]

_{Reviews (1): Last reviewed commit: "Add changelog for Pro doctor specifier s..." | Re-trigger Greptile}

Review: fix: detect stale Pro migration module specifiers

Overall this is a clean, well-scoped addition. The two new regex constants are logically correct and the test structure follows the existing pattern. A few things worth addressing before merge:

🔴 Warning message doesn't cover the new detection types

The existing warning text (unchanged in this PR) was written only for import/require:

⚠️  Found imports from 'react-on-rails' instead of 'react-on-rails-pro':
  • app/javascript/packs/app.test.ts

Fix: Update imports to use 'react-on-rails-pro':
  import ReactOnRails from 'react-on-rails-pro';        // server
  import ReactOnRails from 'react-on-rails-pro/client';  // client

When the trigger is a Jest mock or a declare module block the fix guidance is wrong — the user is told to change an import statement, but there is no import to change. They need to know to update the mock or the type augmentation instead. Suggested fix:

Fix: Replace all base-package references with 'react-on-rails-pro':
  import ReactOnRails from 'react-on-rails-pro';              // ESM import (server)
  import ReactOnRails from 'react-on-rails-pro/client';       // ESM import (client)
  jest.mock('react-on-rails-pro', ...);                       // test mock
  declare module 'react-on-rails-pro' { ... }                 // TypeScript augmentation

🟡 Test gaps

1. No negative test for the mock pattern — jest.mock('react-on-rails-pro', ...) must not trigger a warning. The regex is correct, but a dedicated spec would make that guarantee explicit (see inline comment on line 2549).

2. Subpath mock not covered — jest.mock('react-on-rails/client') is a natural counterpart to the import ... from 'react-on-rails/client' test that already exists.

🟢 Things that look good

• Both new regex patterns correctly exclude react-on-rails-pro — the (?:/[^'"]*) optional subpath group requires a / prefix, so -pro is never a false match.
• ^ in BASE_PACKAGE_DECLARE_MODULE_PATTERN is line-anchored in Ruby (not string-anchored), so multiline .d.ts content is handled correctly.
• .d.ts files are covered implicitly by **/*.ts (they end in .ts); this works as intended, just non-obvious to a future reader.
• The rubocop disables are accurate — Metrics/PerceivedComplexity is the right addition alongside the pre-existing CyclomaticComplexity disable.
• Changelog entry is clear and links the issue correctly.

Code Review

Overall: A focused, low-risk improvement to the Pro migration diagnostic. The two new regex patterns correctly catch categories of base-package references that were previously invisible to the scanner, and the specs cover both new paths. A few things worth addressing before merge:

Warning message is stale for the new pattern types

The existing warning text (⚠️ Found imports from 'react-on-rails'...) and the fix guidance (import ReactOnRails from 'react-on-rails-pro') were written for import/require cases only. Now that the scanner also catches Jest mocks and declare module blocks, the message can mislead users:

• A test file with jest.mock('react-on-rails', ...) doesn't need a new import statement — it needs the mock target updated to 'react-on-rails-pro', or the mock deleted.
• A .d.ts declaration file with declare module 'react-on-rails' needs the block deleted or retargeted, not an import added.

The success path also reads "No base 'react-on-rails' imports found" — technically wrong now that mocks and type declarations are in scope.

Suggested language update (in check_base_package_imports, around line 2778):

checker.add_success("✅ No base 'react-on-rails' references found (Pro package used correctly)")
# ...
checker.add_warning(<<~MSG.strip)
  ⚠️  Found references to 'react-on-rails' instead of 'react-on-rails-pro':
  #{files_with_base_import.map { |f| "  • #{f}" }.join("\n")}

  The base package is a transitive dependency of Pro, so these references resolve
  silently but load the base version without Pro features.

  Fix: replace base-package references with their Pro equivalents:
    import ReactOnRails from 'react-on-rails-pro';         // ES import (server)
    import ReactOnRails from 'react-on-rails-pro/client';  // ES import (client)
    jest.mock('react-on-rails-pro', ...)                   // Jest/Vitest mock
    // delete stale 'declare module' blocks in .d.ts files
MSG

declare module test file is not covered by the glob

The test creates app/javascript/types/react-on-rails.d.ts, and *.ts does match .d.ts files (glob * matches .), so the test works today. Worth a brief note in the test or a comment on the constant that .d.ts files are intentionally included via the *.ts glob — this isn't obvious to future readers and could be accidentally broken if someone restricts the glob to exclude type-declaration files.

No negative regression test for declare module 'react-on-rails-pro'

There's a negative test for imports from 'react-on-rails-pro' but not one for declare module 'react-on-rails-pro' { ... }. Given that the DECLARE_MODULE_PATTERN regex uses a lookahead-free negative approach (the package name must match exactly), a quick sanity spec confirming declare module 'react-on-rails-pro' does not trigger would be a cheap safety net.

Minor: requireActual semantics differ from mock

jest.requireActual('react-on-rails') is used to bypass a mock and explicitly load the real base package — intentional usage, not always a migration leftover. The current implementation flags it (which is defensible as "any base-package reference is suspect"), but if a real user hits a false positive here, the warning could be confusing. At minimum, consider documenting the decision in the constant's comment.

Code Review

Summary

This PR extends react_on_rails:doctor's Pro migration detection to catch two new categories of stale react-on-rails references: Jest/Vitest mock helpers and TypeScript declare module augmentations. The implementation is clean and the test coverage is solid. A few observations below.

Regex correctness ✅

Both new patterns correctly exclude react-on-rails-pro matches:

• react-on-rails(?:/[^'"]*)?['"] ends with a quote, so react-on-rails-pro (which has -pro after the package name) won't match.
• BASE_PACKAGE_DECLARE_MODULE_PATTERN uses ^ which is a Ruby line anchor — correct for multi-line file content.

File extension coverage ✅

.d.ts files are correctly picked up by **/*.ts glob since react-on-rails.d.ts ends in .ts.

Naming inconsistency ⚠️

The method is still named check_base_package_imports and called that way at line 2719, even though every internal variable was renamed to *reference*. See inline comment.

unmock/dontMock in pattern ⚠️

Including these in BASE_PACKAGE_MOCK_PATTERN is defensible for migration diagnostics, but the warning message doesn't distinguish them from mock calls, which could be confusing. See inline comment.

Missing Vitest test case

vi.mock('react-on-rails') works because \w+ matches vi, but there's no spec exercising this path. See inline comment for a suggested test case.

No other issues found

• No security implications — reads files under the project source path only.
• Performance is unchanged — same O(n files) scan, just additional .match? calls per file (fast due to early exit on first match).
• The updated user-facing message and fix guidance are clear and accurate.
• Negative tests (Pro mock / Pro declare module → success) correctly validate the happy path.

Code Review — PR #3232: detect stale Pro migration module specifiers

Overall: Clean, well-scoped change. The new patterns close a real gap (jest/vitest mocks and TS declare module blocks are common Pro migration leftovers), the test matrix is comprehensive for the happy-path cases, and the stress-test.md reformatting is pure noise-free style.

A few things worth addressing before merge:

🐛 Potential issue — BASE_PACKAGE_MOCK_METHODS defined as Regexp but only used as .source

BASE_PACKAGE_MOCK_METHODS = /mock|unmock|doMock|dontMock|requireActual|requireMock|importActual|importMock/
BASE_PACKAGE_MOCK_PATTERN =
  %r{\b(?:\w+\.)?(?:#{BASE_PACKAGE_MOCK_METHODS.source})\s*\(\s*['"]react-on-rails(?:/[^'"]*)?['"]}

BASE_PACKAGE_MOCK_METHODS is a Regexp object but the only use of it is .source to embed the raw string in another pattern. This is subtle: a future maintainer might call .match? against it directly (thinking it's already a complete pattern), or add flags to it (e.g. IGNORECASE) expecting them to carry over into BASE_PACKAGE_MOCK_PATTERN — neither would work. A plain string constant makes the intent unambiguous:

BASE_PACKAGE_MOCK_METHOD_NAMES = "mock|unmock|doMock|dontMock|requireActual|requireMock|importActual|importMock"
BASE_PACKAGE_MOCK_PATTERN =
  %r{\b(?:\w+\.)?(?:#{BASE_PACKAGE_MOCK_METHOD_NAMES})\s*\(\s*['"]react-on-rails(?:/[^'"]*)?['"]}

🧪 Test coverage gap — 5 of 8 listed mock methods have no test

The BASE_PACKAGE_MOCK_METHODS alternation names 8 methods, but the spec only exercises 3:

• ✅ mock (jest.mock)
• ✅ importActual (vi.importActual)
• ✅ importMock (standalone)
• ❌ unmock, doMock, dontMock, requireActual, requireMock

These are real Jest APIs that appear in stale Pro migration artifacts (e.g. jest.requireActual('react-on-rails') is a common pattern). Worth adding at least one spec for a method from each "missing" group, or adding a comment acknowledging the gap is intentional.

🧪 Weak assertion in TypeScript augmentation spec

At spec/lib/react_on_rails/doctor_spec.rb:2627, the test checks:

expect(warning_msgs.any? { |m| m[:content].include?("TypeScript augmentation") }).to be true

But "TypeScript augmentation" appears in the fix-hint block of every warning (regardless of which pattern triggered), so this assertion would pass even if BASE_PACKAGE_DECLARE_MODULE_PATTERN was completely broken and another pattern happened to fire. The file-name check on the line above is the only assertion that validates the TS-specific detection. Consider dropping the second assertion (it doesn't add signal) or replacing it with something like m[:content].include?("declare module").

Minor — warning always lists all three fix hints

The warning message unconditionally shows all three fix examples (ES import, Jest mock, TypeScript augmentation) regardless of which pattern fired. For a jest.mock violation, showing "declare module 'react-on-rails-pro' { ... } // TypeScript augmentation" is slightly confusing. Low priority, but worth considering filtering hints based on what matched.

Pre-existing — .mjs / .cts / .mts extensions not scanned

js_extensions = %w[js jsx ts tsx] doesn't cover modern ESM/CTS variants. Vitest mock files frequently use .mts. Not introduced by this PR, but relevant now that mock patterns are in scope.

Code Review - PR 3232

Overall: Clean, well-scoped improvement. The refactoring from an imperative each-loop to flat_map+select reads better, and the decomposition into helper methods is a clear win for testability. Test coverage is thorough with positive and negative cases for every new pattern.

What the PR does

Extends react_on_rails:doctor Pro-migration check to catch two additional forms of stale react-on-rails base-package references: Jest/Vitest mock helpers and TypeScript declare module augmentations in .ts/.d.ts files.

Specific notes

BASE_PACKAGE_MOCK_METHOD_NAMES should be frozen. It is only consumed at class-load time when BASE_PACKAGE_MOCK_PATTERN is built via string interpolation, so leaving it mutable is inconsistent — BASE_PACKAGE_REFERENCE_PATTERNS is explicitly .freezed. Either freeze it, or inline the string directly into the regex literal and remove the intermediate constant entirely since it has no other use.

BASE_PACKAGE_MOCK_PATTERN is intentionally broad. The regex matches any function named mock etc. regardless of receiver (not just jest/vi). False positives are practically impossible given how Jest/Vitest-specific the name list is (requireActual, dontMock, importActual), but a short inline comment noting the intentional broadness would help future readers.

No duplicates from flat_map — confirmed correct. Extensions are mutually exclusive so a file can never appear in two glob results.

.d.ts coverage is correct. Ruby Dir.glob matches foo.d.ts for **/*.ts. The clarifying comment in the code is appreciated.

Minor nit: vi.mock is missing from the fix example in the warning message. The message lists jest.mock(react-on-rails-pro, ...) as the recommended replacement but does not show the Vitest equivalent. Since the PR explicitly adds Vitest support, adding vi.mock('react-on-rails-pro', ...) to the suggestion block would make it complete for Vitest users.

Summary

Implementation is correct and tests are solid. Two actionable items before merge: (1) freeze BASE_PACKAGE_MOCK_METHOD_NAMES, and (2) add vi.mock to the warning fix example.

Address-review summary

Scan scope: default cutoff (previous summary at 2026-05-16T16:02:45Z). Covers the claude-review pass on the postfix-guard fix (60ca5c3). Nothing actionable.

Mattered

• None. The postfix-guard fix (60ca5c3) was empirically validated: gem(...) declarations with postfix modifiers (if/unless/and/defined?), ) inside strings, trailing comments, multiline, and no-trailing-newline variants all produce valid Ruby with pins/options preserved (15+ adversarial cases). An initially-suspected adjacent "comment before the version comma" corruption was reproduced as garbage-in: that input is invalid Ruby before the generator runs, so it is not a generator defect. No code change.

Skipped

• pro_migration.rb:143 #-in-string (3253321272): re-raise of an empirically-refuted item; realistic git#ref/path#/multiline forms reproduced as valid this round. Resolved.
• pro_migration.rb:80 tri-state return (3253321423): clarity observation, not a bug; declined multiple times; fully tested. Resolved.
• pro_generator.rb:924 no-trailing-newline missed ) (3253321579): empirically refuted; all no-newline cases produce valid Ruby; the one invalid-output case has invalid-Ruby input (GIGO). Resolved.
• pro_generator.rb:869 backtick version string (3253321741): Minor/speculative; backtick is shell-exec in Ruby, not a Gemfile version literal; pre-existing code untouched by this fix. Resolved.

General overview comment 4467840026 is a non-actionable aggregate.

Note: the only red check is markdown-link-check (pre-existing dead link in PROJECTS.md, unrelated to this .rb-only PR, tracked in #3306).

No follow-up issue. Next default scan starts after this comment. Say check all reviews to rescan the full PR.

Code Review: PR #3232 — Fix stale references and lost Gemfile pins after Pro migration

Overview

This PR does three things:

1. Extends the Pro generator rewriter to handle jest.mock/vi.mock, requireActual/importActual, TypeScript declare module, and side-effect import 'react-on-rails' patterns — previously only import/require/dynamic-import were rewritten.
2. Preserves the user's Gemfile version pin (and other gem options like path:, git:, require:) when swapping react_on_rails → react_on_rails_pro — previously the pin was overwritten with the running gem's version.
3. Widens react_on_rails:doctor to detect all of the above patterns (plus commented-out references), acting as a superset safety net that covers hand-written code added post-migration.

The approach is well-thought-out: the generator uses a conservative allowlist for destructive rewrites while the doctor uses broader detection (intentionally including comments, as documented in the warning message).

Correctness

Gemfile parsing (ProMigration module): The multi-phase parser for both parenthesized and non-parenthesized gem declarations is correct and well-tested. Handling of if: ENV[...] conditionals, trailing comments, inline comments between arguments, and multi-line declarations all appear correct.

Version pin detection (has_user_version_pin at pro_generator.rb:869): The regex /\A\s*,\s*(?:#[^\n]*\n\s*)*["']/ correctly identifies a string literal (version) after the comma, distinguishing it from keyword args like path:, git:, require:. This is the core fix for the pin-preservation bug. ✓

Regex false-positive check: The doctor's BASE_PACKAGE_IMPORT_PATTERN = %r{\bfrom\s+['"]react-on-rails(?:/[^'"]*)?['"]} does NOT false-positive on react-on-rails-pro because after matching react-on-rails, the optional (?:/...) group requires a / prefix, and the closing ['"] cannot match - in -pro. ✓

Template literal handling (rewrite_outside_inline_template_literals): The placeholder approach uses the hard-coded prefix __ROR_TEMPLATE_LITERAL_PLACEHOLDER_N__. If a user's JS file literally contains that string, it would be corrupted during rewriting. This is astronomically unlikely but worth a note.

Issues Found

1. Silent removal of duplicate gem 'react_on_rails' declarations (minor bug)

swap_base_gem_for_pro_in_gemfile (pro_generator.rb:113-139) sets pro_entry_added = true after the first replacement, causing any subsequent react_on_rails gem line to be silently dropped rather than replaced. While two gem declarations in one Gemfile is unusual, Bundler does allow them in group blocks. Silently dropping a line is worse than emitting a warning.

Suggestion: Add a warning when a second base gem entry is encountered after pro_entry_added is true.

2. Misleading rescue message in rollback_gemfile_after_failed_swap_precondition (pro_generator.rb:435)

The rescue block says "Gemfile remains updated with react_on_rails_pro." but this method handles precondition failures, not post-swap failures. In the context where this fires (prerequisites auto-installed via bundle add but no base gem was found to swap), the phrase reads as if a swap already happened when it may not have. The method name and rescue message are slightly at odds.

3. Generator reads JS files without encoding validation (pro_generator.rb:274)

content = File.read(file)

The doctor's equivalent uses File.binread(file).force_encoding("UTF-8") then content.valid_encoding? before processing (doctor.rb:2982-2983). The generator skips this check. While the per-file rescue StandardError at line 286 catches encoding errors that propagate during file reading, some malformed byte sequences only surface during regex matching. Consider mirroring the doctor's binary-read approach.

4. Inconsistent STRING_LITERAL_PATTERN between shared module and generator

ProMigration::STRING_LITERAL_PATTERN (pro_migration.rb:23) uses possessive quantifiers (*+) for performance. The generator's local line_without_string_literals_and_inline_comments (pro_generator.rb:668-676) uses regular * quantifiers. Ruby's Oniguruma supports possessive quantifiers so this is not a bug, but the shared constant could simply be reused for consistency.

5. strip_ruby_inline_comment is fragile on strings containing # (pro_migration.rb:166)

line_continues_with_comma? calls strip_ruby_inline_comment directly without first stripping string literals. A Gemfile line like:

  path: "../path#with-hash",

would incorrectly treat #with-hash", as a comment and mark the line as ending with a comma. In practice Gemfile argument values rarely contain #, but it is a correctness gap. The safer approach would be to use line_without_string_literals_and_inline_comments first, as is already done in match_multiline_parenthesized_base_gem.

Doctor: Intentional Design Choices Worth Acknowledging

• Includes commented references: The warning explicitly states "this includes commented-out references". This is intentional and correct — commented references indicate incomplete migrations.
• Runs only under Pro: check_base_package_references is guarded by ReactOnRails::Utils.react_on_rails_pro?. A project that has migrated manually without the Pro gem loaded won't get these warnings from the doctor, which is consistent with the current doctor architecture.

Test Coverage

The test coverage is very comprehensive (2,003 lines for the generator spec, 4,215 for the doctor spec). Specific scenarios tested include:

• All new rewriter patterns (jest/vitest mock, declare module, side-effect imports)
• Multiline parenthesized gem declarations with version pins, path:, git:, comments
• Rollback on failed bundle install
• File-disappears-during-scan resilience
• Custom Shakapacker source_path detection
• TypeScript export declare module form

Missing test: duplicate react_on_rails gem declarations (the silent-drop issue from #1 above).

Summary

This is solid, well-tested work that fixes a real user pain point. The Gemfile parsing logic is particularly careful. The main issues are: (1) silent line-dropping on duplicate gem declarations (minor correctness gap), (2) the encoding handling mismatch between generator and doctor, and (3) the fragile strip_ruby_inline_comment usage. None of these are showstoppers, but issue #1 is worth addressing before merge.

Code Review — PR #3232

Overview

This PR resolves issue #3104 by (1) extending the Pro migration generator to rewrite Jest/Vitest mock helpers and TypeScript declare module augmentations alongside existing import/require rewrites, (2) preserving the user's existing Gemfile version pin (and other gem options) during the react_on_rails → react_on_rails_pro swap instead of overwriting it, and (3) widening react_on_rails:doctor to flag the same patterns the rewriter handles plus side-effect imports.

The extraction of shared constants and Gemfile-parsing logic into a new ProMigration module is a clean DRY improvement. Test coverage is broad and well-organised.

Bugs

1. Extra blank line introduced in Gemfile for multiline parenthesised declarations

remove_parenthesized_gem_call_closing_parenthesis (line 894 of pro_generator.rb) returns:

"#{prefix}#{rest}"

When the closing ) is on its own line (the common Bundler style), prefix already ends with \n (the last argument line) and rest is \n (the newline after )). The result is a double \n, which injects a blank line into the Gemfile.

Reproduction: any case with gem(\n "react_on_rails",\n "~> 16.0"\n)\n.

None of the multi-line parenthesised tests use eq for the full result — they use include matchers and so miss this. The postfix-code branch already handles trailing whitespace correctly with rstrip/lstrip; the no-postfix-code branch needs the same treatment:

# current (inserts double \n)
"#{prefix}#{rest}"
# fix
"#{prefix.chomp}#{rest}"

Add at least one eq-style assertion covering a basic multi-line parenthesised replacement (no postfix guard) to lock this in.

2. Offset from original match used against rewritten string

In rewrite_statement_suffix_after_pending_module_specifier (line 609):

closing_quote_index = line.index(pending_match[:quote], pending_match.end(0))

line is the rewritten string (after the sub that replaced react-on-rails with react-on-rails-pro), but pending_match was captured from the original string. The replacement adds 4 bytes (-pro), so pending_match.end(0) falls 4 bytes before where the closing quote actually is in the rewritten string.

The search still finds the correct character because neither package name contains the quote character — a forward search from an earlier offset lands on the right quote. But this relies on an implicit constraint that is easy to break. A comment documenting the assumption, or recording the offset delta after the sub, would protect future editors.

Design / Behaviour Concerns

3. When react_on_rails_pro is already present, base gem entries are left in place and bundle_install is not called

Old behaviour: the base gem entry was removed; bundle_install_after_gem_swap was called so Bundler updated its lock.
New behaviour: if pro_gem_entry? is true the Gemfile is returned completely unchanged (both entries remain) and no install runs.

Users who manually added react_on_rails_pro and re-run the generator are left with a stale react_on_rails entry that will load the base package at boot (the very silent-failure the PR is trying to prevent). If keeping both entries is the deliberate policy the warning message should explicitly tell users they need to remove the base gem entry themselves, and the doctor check should flag this Gemfile state.

Minor Observations

Sentinel value false in parenthesized_base_gem_name: The method returns nil (not yet found), a Hash (base gem found), or false (non-base gem found — stop). The ||= propagation is correct, but false as a sentinel interacts subtly with ||= and will surprise maintainers. A named constant (:not_base_gem) or a dedicated two-field struct would be clearer.

PRO_GEM_PATTERN vs. the iterative parser: PRO_GEM_PATTERN handles only comment-line continuations between gem( and the gem name ((?:#.*\n\s*)*), whereas match_multiline_parenthesized_base_gem also handles blank lines and non-comment continuations. For detection-only use this is unlikely to matter in practice, but the asymmetry is worth a comment.

Doctor warning on commented-out code: Explicitly noted in the PR and the message now says so — just confirming it is prominent enough since it is the first thing users will act on.

Test Coverage Gaps

• No eq-based assertion for a multi-line parenthesised replacement without a postfix guard (needed to catch the double-\n regression above).
• No unit test for rewrite_statement_suffix_after_pending_module_specifier in isolation — the method exists because of a real edge case and deserves its own test.
• The "leaves base gem entries unchanged when react_on_rails_pro already exists" test should assert that the base gem entry is still present in the output Gemfile so the policy is explicit to future readers.

Summary

The core feature (version-pin preservation, extended rewriter, wider doctor) is solid and well-tested. Two items worth addressing before merge:

1. Extra blank line in parenthesised Gemfile declarations — "#{prefix.chomp}#{rest}" plus an eq-style test.
2. Policy clarification for the case where react_on_rails_pro already exists in the Gemfile — either remove the stale base entry (restoring previous safe behaviour) or make the warning much louder and have the doctor flag the mixed-Gemfile state.

Code Review — PR #3232 (fix: stale references and lost Gemfile pins after Pro migration)

Overview

This PR fixes two genuine user-facing bugs (stale JS references after Pro migration and overwritten Gemfile version pins) by: extracting shared logic into a new ProMigration module, widening the JS rewriter to cover Jest/Vitest mocks and TypeScript declare module blocks, and flipping the Gemfile swap from "replace the whole gem line" to "preserve the user's suffix, inject the default version only when none is present."

The approach is sound and well-targeted. Test coverage is exceptional — particularly the parametric multi-helper table test, the use of Ripper.sexp to validate that rewritten Gemfiles are parseable Ruby, and the thorough positive/negative fixture pairs in doctor_spec.rb.

Correctness

Gemfile version-pin preservation (build_pro_gem_replacement_line)

The has_user_version_pin heuristic — "suffix begins with , optionally followed by Ruby comments, then a string quote" — is a clean way to distinguish a version string from keyword arguments. It correctly handles:

• gem "react_on_rails", "~> 16.0" (preserves pin)
• gem "react_on_rails", require: false (adds default version)
• gem "react_on_rails", ">= 15.0", "< 16.0" (preserves both constraints)
• gem("react_on_rails", ,) (trailing-comma degenerate → normalised to \n)

New rewrite patterns

The pattern set (MOCK_CALL_PATTERN, DECLARE_MODULE_PATTERN, SIDE_EFFECT_IMPORT_PATTERN) correctly avoids false positives on the Pro package because the structural lookahead (?=(?:["']|/)) already rejects -pro (a hyphen is neither a quote nor /). The explicit (?!-pro) in each pattern is a belt-and-suspenders safeguard that makes the intent readable.

Doctor / generator symmetry

The doctor intentionally scans broader than the rewriter (e.g. bare importActual/importMock, comments containing the package string). The comment in BASE_PACKAGE_MOCK_PATTERN acknowledges this is a "deliberately broad detector heuristic." This asymmetry is correct: detection is advisory, mutation is destructive.

Issues

1. Implicit invariant in rewrite_statement_suffix_after_pending_module_specifier

The method receives the rewritten line but the original MatchData object:

closing_quote_index = line.index(pending_match[:quote], pending_match.end(0))

pending_match.end(0) is the end position in the original string. In the rewritten string the replacement added -pro (4 characters), so the closing quote now sits 4 characters further right. String#index(needle, start) performs a forward search, so as long as the 4 inserted characters (-pro) contain no quote characters this happens to find the right quote. That invariant holds today, but the code is silently relying on it. See inline comment for a suggested fix.

2. Tri-value sentinel (nil / false / Hash) in parenthesized_base_gem_name

The function returns nil (undecided — keep scanning), false (found a non-base-gem parenthesized call — stop), or a Hash (found the base gem). The caller uses ||= to guard re-assignment, but ||= re-evaluates when the current value is false (falsy). It works correctly only because return nil if gem_name == false appears on the very next line after the assignment. A brief comment would prevent future confusion. See inline comment.

3. normalize_suffix = "\n" if ... lacks context

The guard normalized_suffix = "\n" if normalized_suffix.match?(/\A,\s*\n\z/) silently converts a trailing-comma-only suffix into a newline. This handles the output of remove_parenthesized_gem_call_closing_parenthesis when the original suffix was ,) (e.g. gem("react_on_rails",)). A one-line comment would clarify why it exists. See inline comment.

Minor observations

• GEMFILE_STRING_DELIMITERS (["'", '"', "\"]) is defined in pro_generator.rbbut conceptually belongs alongsideSTRING_LITERAL_PATTERNinProMigration`. Low priority since it is only used in the generator today.
• base_gem_entry? uses a while / line_index += 1 loop where lines.each_with_index would be more idiomatic; the current form is readable enough given the existing style of the file.
• Tests that use Dir.chdir inside around blocks work fine serially but may interact badly if the suite is ever run with --parallel. Pre-existing pattern, not introduced here.

Security / Performance

No concerns. Regex patterns use possessive quantifiers (*+) via Oniguruma where applicable to prevent catastrophic backtracking. File I/O uses File.binread + encoding validation to safely skip binary files. The node_modules/ exclusion filter prevents the scanner from walking vendored packages.

Verdict: Approve with the three inline comment suggestions addressed (two are clarity/safety notes; none are blocking bugs). The PR is a net improvement on every dimension.

Address-review summary — codex/doctor-pro-module-specifiers @ fa798dc54

MUST-FIX (1) — addressed

• Stale-base regression. When a react_on_rails_pro entry already existed in the Gemfile, the swap preserved the now-stale gem "react_on_rails" line and skipped bundle install, re-introducing the exact react_on_rails:pro auto-upgrade: import rewriter misses jest.mock and declare module; Gemfile swap overwrites version pins #3104 stale-reference state and diverging from main. Root cause traced to a134cf505, which conflated the static has_pro_gem_entry precondition with the running pro_entry_added state. Fixed in fa798dc54: the swap now drops the stale base, runs bundle install, and reports the removal. Empirically validated against the real generator across a pro+base / if-else / group / parenthesized / postfix / no-pro matrix — every valid input produces output that passes both ruby -c and Bundler::Dsl.evaluate. (Raised out-of-band; no GitHub thread.)

Resolved review threads

• r3253425323 — double \n produced a blank line after a multiline parenthesized declaration. Fixed via prefix.chomp in fa798dc54; independently shown to also improve the trailing ) # comment and no-final-newline cases. Replied + resolved.
• r3253425415 — pending_match offset fragility. Skipped: the code is correct by construction (the early-offset window only ever contains react-on-rails-pro, and package names cannot contain quote characters), so there is no reachable defect; the requested explanatory comment is a defensive annotation we deliberately avoid. Rationale replied + resolved.

Spec changes (in fa798dc54)

• Rewrote the two tests that asserted the old preserve-base behavior to assert the corrected remove-base + bundle install behavior, with byte-exact eq expectations reproduced from the real generator.
• Added a byte-exact eq test asserting no blank line follows a multiline parenthesized declaration.
• Reworked the duplicate-declaration test to use a Bundler-valid Gemfile (the prior input was itself Bundler::Dsl::DSLError), so it no longer defends behavior with invalid input.

All review threads on this PR are now resolved (0 unresolved). No check all reviews override was used; future runs should triage feedback posted after this comment.

Address-review summary

Scan scope: default cutoff was the prior checkpoint at 2026-05-17T17:13:27Z, but the new claude-review burst straddled it (threads at 17:13:14Z, 17:13:22Z, 17:13:29Z — two predate the checkpoint). Scanned all unresolved threads instead, reliable here because every earlier thread was resolved before that checkpoint. No check all reviews override used.

Mattered

• None. 0 MUST-FIX, 0 DISCUSS. Each of the three new claude[bot] comments was verified against the actual code and found correct; all asks were comment-only (or an optional refactor).

Skipped

• pro_generator.rb:608 (r3255118936) — re-raise of the pending_match offset "silent invariant". Duplicate of already-resolved thread 3253425415 (declined with maintainer approval). Correct by construction: the early-offset window only ever contains react-on-rails-pro, and package names cannot contain quotes. The suggested re-match restructuring is self-documentation, not a fix. Rationale replied + resolved.
• pro_migration.rb:80 (r3255119251) — request to comment the nil/false/Hash return contract. Verified: the caller's return nil if gem_name == false runs on the line immediately after the ||=, so the falsy-false re-eval the comment warns about is unreachable. Explanatory-comment request for correct code. Rationale replied + resolved.
• pro_generator.rb:880 (r3255119420) — request to comment the trailing-comma guard. Verified correct and covered by the parenthesized/postfix validation matrix. Restate-the-mechanism comment request. Rationale replied + resolved.

All review threads on this PR are now resolved (0 unresolved). Next default scan starts after this comment; say check all reviews to rescan the full PR.